fix(agent): DeepSeek thinking-mode reasoning_content invariant + skill-creator hard-gate false-positive cleanup (#146 follow-up)#164
Merged
ZhouChaunge merged 6 commits intoMay 26, 2026
Conversation
…riant DeepSeek's thinking-mode protocol requires that once ANY assistant message in history carries a non-empty 'reasoning_content', every subsequent assistant message must also carry one — otherwise the API returns: 400 'reasoning_content in the thinking mode must be passed back to the API' This hit users whenever: 1) A synthetic skill_invoke -> read_file assistant message was injected mid-turn after a real thinking-mode assistant message (agent-loop.js injectSyntheticSkillRead). 2) The force-final-summary fallback at iteration-limit produced an assistant message that lost its thoughts stream. Changes: - agent-loop.js: synthetic skill-read assistant now carries a short reasoning_content; force-final-summary captures tailThoughts and attaches them so reload/round-trip never lands on a reasoning-less assistant. - providers/index.js sanitizeMessages: defence-in-depth backfill. Once any assistant in history has reasoning_content, every other assistant without one gets a short placeholder. The pass is scoped to reasoning-capable models only and runs after the strip pass.
…talled (#146 follow-up) Issue #146 introduced a 'HARD GATE' requiring skill_invoke({name:'skill-creator'}) before any skill_create call. In environments where no skill-creator meta-skill is actually installed, the model still followed the instruction and fabricated a skill_invoke for the non-existent skill, producing a confusing: Error: skill 'skill-creator' not found. Available: ... Three coordinated changes silence the false positive: - prompts/system.js: only emit the gate paragraph if discoverSkills() reports a name in the {skill-creator, skill_creator, skillcreator} variant set is actually installed in the current workspace. - tools/schema.js: soften skill_create description from 'HARD GATE' to 'Quality gate — ONLY if a meta-skill named ... is listed in the Available skills index'; explicitly tells the model not to fabricate the invoke when the gate is inactive. - tools/skill-tools.js: when the model still fabricates a call to one of the variant names, return a soft Notice (gate inactive, you may proceed) instead of an error. Executor-side gate is already a no-op when no meta-skill is installed, so this is consistent end-to-end.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| reasoning_content: `Loading skill SOP "${safeName}" via read_file to obtain its instructions before proceeding.`, | ||
| tool_calls: [{ | ||
| id: callId, | ||
| type: 'function', |
There was a problem hiding this comment.
在这里，reasoning_content 的内容是动态生成的，需确保 safeName 的来源是安全的，避免潜在的命令注入或信息泄露风险。此外，建议对 reasoning_content 进行适当的转义处理，以防止恶意输入导致的安全问题。
| }); | ||
| } | ||
| } catch (e) { | ||
| // Restore the last known-good message state to prevent persisting a |
There was a problem hiding this comment.
在 onThinking 的回调中，tailThoughts 变量的使用需要确保在多线程环境下不会出现竞态条件。建议在对 tailThoughts 进行操作时加锁，确保线程安全。此外，catch 语句中仅记录了错误信息，建议在捕获异常后进行适当的处理，以防止潜在的未处理异常导致的系统不稳定。
|
|
||
| return `# Problem-solving paradigm | ||
|
|
||
| For any non-trivial task, follow this loop: |
There was a problem hiding this comment.
- 安全性: 在调用
discoverSkills函数时，未对返回值进行充分的验证，可能导致未处理的异常或错误数据。建议在使用all变量之前，确保其内容符合预期。 - 异常处理:
catch语句未指定异常类型，可能会掩盖其他潜在的错误。建议至少记录错误信息以便于调试。 - 代码风格: 代码中使用了较多的注释，虽然有助于理解，但建议将注释内容简化并保持一致性，以符合项目的整体风格。
- 性能: 使用
Set来存储变体名称是合理的，但在some方法中进行字符串转换可能会影响性能，尤其是在技能数量较多时。可以考虑优化此部分。
| Never call \`skill_create\` for one-off fixes, trivial tasks, or before the user has confirmed the solution works.${gateParagraph}`; | ||
| } | ||
|
|
||
| // ---------- workspace instructions (lazy, opt-in) ---------- |
There was a problem hiding this comment.
- 安全性: 直接拼接
gateParagraph可能导致潜在的注入风险，建议使用模板字符串或其他安全方式来构建字符串。 - 可维护性: 代码逻辑较为复杂，建议将技能检查逻辑提取为单独的函数，以提高可读性和可维护性。
- 一致性: 在注释中提到的
skill_create的使用说明与之前的逻辑相矛盾，可能会导致用户混淆。建议统一说明，确保用户理解何时调用skill_create。
| return out; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
-
安全性: 在处理
messages时，需确保输入的每个消息对象都经过严格验证，避免潜在的对象注入攻击。建议在sanitizeMessages函数中增加对messages中每个对象的结构和类型的验证。 -
异常处理: 当前代码中未对
getProvider和getModel的返回值进行有效性检查，若返回undefined或null，可能导致后续代码抛出异常。建议在使用这些函数的返回值前进行检查。 -
代码风格: 代码中使用了
Object.prototype.hasOwnProperty.call，建议统一使用Object.hasOwn，以提高可读性和一致性。 -
性能: 在
out.map的实现中，如果effectiveStrip为空，仍然会执行一次map，可以考虑在此处添加条件判断，避免不必要的遍历。 -
可维护性: 代码中对
reasoning_content的处理逻辑较为复杂，建议将其提取为单独的函数，以提高代码的可读性和可维护性。
| description: 'Persist a reusable multi-step workflow as a new skill on disk. Use ONLY when ALL of these are true: (1) you have just completed a non-trivial multi-step task, (2) the user has explicitly confirmed the result is correct, (3) the workflow has ≥3 concrete steps that span multiple tools and is likely to recur. Do NOT use for one-off fixes, trivial edits, single-step tasks, before user confirmation, or to record preferences (those belong in ~/.deepcopilot/memory.md) or project facts (those belong in <workspace>/DEEPCOPILOT.md). When the SOP was synthesized from web research, set source to "web" or "hybrid" — the skill will be marked untrusted automatically. **Quality gate (Issue #146)**: ONLY if a meta-skill named `skill-creator`, `skill_creator`, or `skillcreator` is listed in the "Available skills" index of the current system prompt, you MUST call `skill_invoke` earlier in the SAME turn with `name` set to that exact installed spelling before calling skill_create. If no such meta-skill appears in the index, do NOT fabricate a `skill_invoke` call for it — just call `skill_create` directly; the gate is inactive in that environment.', | ||
| parameters: { | ||
| type: 'object', | ||
| properties: { |
There was a problem hiding this comment.
在描述中，'HARD GATE' 被修改为 'Quality gate'，这可能会导致对技能调用的理解产生混淆。建议保持原有的术语，以确保开发者在使用时不会误解。此外，描述中提到的技能调用逻辑需要确保在实现时有充分的验证和错误处理，以防止潜在的命令注入或路径穿越等安全漏洞。建议在调用 skill_create 之前，增加对输入参数的校验，确保其安全性和有效性。
| } | ||
| const known = all.map(x => x.name).join(', ') || '(none installed)'; | ||
| return `Error: skill "${name}" not found. Available: ${known}`; | ||
| } |
There was a problem hiding this comment.
在这里添加了对未安装的技能创建者元技能的处理逻辑，虽然可以减少用户的困惑，但需要注意以下几点：
-
安全性：返回的通知信息中包含了关于环境状态的详细信息，可能会暴露系统的内部实现细节，建议使用更通用的消息，避免泄露敏感信息。
-
异常处理：当前实现没有对
name参数进行有效性检查，若name为null或undefined，可能会导致后续的toLowerCase()调用抛出异常。建议在使用前进行检查。 -
代码风格：建议在代码中添加更多的注释，尤其是对逻辑的解释，以便后续维护人员理解。
-
可维护性：考虑将技能名称的变体提取到常量中，以便于后续的修改和维护。
综上所述，建议对该部分代码进行修改，以提高安全性和可维护性。
There was a problem hiding this comment.
Pull request overview
This PR addresses two user-visible issues in DeepCopilot’s agent loop and skill system: (A) DeepSeek “thinking mode” sessions failing with a reasoning_content round-trip invariant error, and (B) a skill-creator “hard gate” instruction causing fabricated skill_invoke calls and confusing “skill not found” failures when the meta-skill isn’t installed.
Changes:
- Preserve / backfill
reasoning_contentto satisfy DeepSeek thinking-mode invariants, including for synthetic skill-read injections and force-final-summary fallback. - Make the
skill-creatorquality gate conditional on actual installation (prompt + schema wording), and softenskillInvokebehavior for fabricatedskill-creatorcalls. - Add defense-in-depth sanitization behavior in provider message preparation.
结论：需修改
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/chat/agent-loop.js | Adds reasoning_content handling for synthetic skill-read injection and iteration-limit force-final-summary. |
| src/providers/index.js | Extends sanitizeMessages() with a reasoning-mode invariant backfill for reasoning_content. |
| src/prompts/system.js | Makes the skill-creator gate paragraph conditional based on locally discovered skills. |
| src/tools/schema.js | Updates skill_create tool description to reflect a conditional “quality gate” and discourage fabrication. |
| src/tools/skill-tools.js | Returns a soft Notice (instead of Error) for fabricated skill-creator skill_invoke calls when missing. |
Comment on lines
+40
to
+41
| if (variants.has(name.toLowerCase())) { | ||
| return 'Notice: no skill-creator meta-skill is installed in this environment. The skill_create quality gate is inactive here — you may proceed to call `skill_create` directly when appropriate.'; |
Comment on lines
65
to
69
| // message), so we must include a non-empty reasoning_content here. | ||
| // Sanitization (sanitizeMessages in providers/index.js) preserves | ||
| // this field for reasoning-capable models, so it survives to the API. | ||
| reasoning_content: `Loading skill SOP "${safeName}" via read_file to obtain its instructions before proceeding.`, | ||
| tool_calls: [{ |
Comment on lines
+194
to
+200
| // Reasoning-mode invariant backfill. Only kicks in once at least one | ||
| // assistant message already carries reasoning_content — that's the | ||
| // moment DeepSeek starts enforcing the "every assistant has it" rule. | ||
| if (isReasoning) { | ||
| const anyHasReasoning = out.some( | ||
| m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, | ||
| ); |
Comment on lines
+276
to
+280
| try { | ||
| const { discoverSkills } = require('../skills'); | ||
| const all = discoverSkills(wsRoot()); | ||
| const variants = new Set(['skill-creator', 'skill_creator', 'skillcreator']); | ||
| skillCreatorInstalled = all.some(s => variants.has(String(s && s.name || '').toLowerCase())); |
Four targeted refinements to the previous two commits, all from Copilot's PR #164 review: 1. agent-loop.js (injectSyntheticSkillRead): only attach reasoning_content when history already shows thinking mode is live. Previously the synthetic value was added unconditionally, which on non-DeepSeek providers (a) sent an unknown field to OpenAI-compatible endpoints, and (b) artificially activated the providers/index.js backfill. The new gate ('has any prior assistant carried non-empty reasoning_content?') exactly matches the moment DeepSeek's API starts enforcing the round-trip rule. 2. providers/index.js (sanitizeMessages): narrow the backfill gate from isReasoning alone to isReasoning AND stripInputFields.includes('reasoning_content'). OpenAI / Anthropic models may set capabilities.reasoning=true without honoring the round-trip protocol; without this tighter check we would have injected reasoning_content into requests bound for endpoints that reject unknown fields. 3. skill-tools.js (skillInvoke): when the requested name is a skill-creator variant but no exact match exists, scan the installed skills (case-insensitive) for a matching creator. If found, return an actionable 'did you mean X?' error so the gate stays effective under wrong casing/spelling. Only return the soft 'gate is inactive' notice when no creator variant is actually installed. 4. system.js (readSkillIndex + getProblemSolvingParadigm): introduce a per-buildSystemPrompt skills cache so discoverSkills() — which walks the filesystem and reads each SKILL.md body — is called once per prompt build instead of twice. Cache is reset at the start of every buildSystemPrompt() so freshly-installed skills appear without a process restart.
Comment on lines
+41
to
+44
| const SKILL_CREATOR_VARIANTS = new Set(['skill-creator', 'skill_creator', 'skillcreator']); | ||
| if (SKILL_CREATOR_VARIANTS.has(name.toLowerCase())) { | ||
| const installedCreator = all.find( | ||
| x => SKILL_CREATOR_VARIANTS.has(String(x && x.name || '').toLowerCase()), |
Comment on lines
+204
to
+213
| // moment DeepSeek starts enforcing the "every assistant has it" rule. | ||
| // Scoped to providers that actually honor the round-trip protocol so we | ||
| // never inject reasoning_content into requests bound for OpenAI/Anthropic. | ||
| if (honorsReasoningRoundTrip) { | ||
| const anyHasReasoning = out.some( | ||
| m => m && m.role === 'assistant' && typeof m.reasoning_content === 'string' && m.reasoning_content.length > 0, | ||
| ); | ||
| if (anyHasReasoning) { | ||
| out = out.map(m => { | ||
| if (!m || m.role !== 'assistant') return m; |
Comment on lines
149
to
153
| @@ -153,27 +153,71 @@ function listModels(providerId) { | |||
| * Exception: `reasoning_content` is never stripped for reasoning-capable models | |||
…riants Three more refinements from Copilot's PR #164 follow-up review: 1. providers/index.js JSDoc: rewrite to clearly distinguish the two branches — non-thinking-mode (strip reasoning_content because DeepSeek 400s) vs. thinking-mode (preserve + backfill because the API requires round-trip). Previous doc conflated them. 2. providers/index.js sanitizeMessages: narrow backfill to messages AFTER the first assistant carrying non-empty reasoning_content. DeepSeek's documented rule is about *subsequent* assistants, not all of them; this keeps pre-thinking history untouched and reduces request payload. 3. skill-tools.js: hoist SKILL_CREATOR_NAMES to module scope above skillInvoke so the soft-notice path and the existing skill_create gate share the same canonical variant set. Removes the duplicated SKILL_CREATOR_VARIANTS literal that risked drifting out of sync.
Comment on lines
+18
to
+22
| // Issue #146 — Meta-skill that must review every skill_create call. | ||
| // Accept a couple of common spelling variants to be permissive about how | ||
| // the on-disk skill is named. Defined at module scope so both skill_invoke | ||
| // (for "did you mean X?" handling) and the skill_create gate share the same | ||
| // canonical variant set. |
Copilot review round 3: the comment above SKILL_CREATOR_NAMES claimed the meta-skill 'must review every skill_create call', but the actual gate is conditional — it only fires when a creator skill is installed in the workspace, matching the conditional prompt paragraph in system.js and the schema wording in schema.js. Rephrase the comment to reflect the runtime behaviour and cross-reference the related call sites.
Comment on lines
146
to
147
| * Returns a NEW array; never mutates the input. The strip rule lives in the | ||
| * provider JSON so a vendor's protocol quirks stay encapsulated. |
Copilot review round 4: JSDoc claimed 'Returns a NEW array', but the implementation may return the original messages reference when no fields are stripped and no backfill applies (the fast path for OpenAI/Anthropic calls with empty stripInputFields). Update the contract to describe both branches, and explicitly tell callers to treat the result as read-only. Behaviour unchanged — the original-reference fast path avoids an unnecessary shallow copy on the hot path.
Comment on lines
+146
to
+151
| * Never mutates the input. Returns either a NEW array (when at least one | ||
| * message was rewritten by stripping or backfill) or the ORIGINAL `messages` | ||
| * reference unchanged (fast path: no quirks apply, e.g. an OpenAI/Anthropic | ||
| * call with an empty `stripInputFields`). Callers must treat the result as | ||
| * read-only — do not mutate elements of the returned array. The strip rule | ||
| * lives in the provider JSON so a vendor's protocol quirks stay encapsulated. |
ZhouChaunge
deleted the
fix/deepseek-thinking-invariant-and-skill-gate-cleanup
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related agent-loop / skill-system bug fixes that were sitting in a working tree after #160 and #163 merged. Both surface as user-visible errors in current main.
Theme A — DeepSeek thinking-mode
reasoning_contentinvariantSymptom: intermittent
400 reasoning_content in the thinking mode must be passed back to the APIfrom DeepSeek after either a mid-turn skill injection or a force-final-summary fallback at the tool-iteration limit.Root cause: DeepSeek enforces the rule "once any assistant message in history carries
reasoning_content, every subsequent assistant message must also carry one". Two code paths produced assistant messages without it after thinking had already started:injectSyntheticSkillReadin src/chat/agent-loop.js — the syntheticread_filetool-call we splice in to load skill SOPs.tailwas captured but the thoughts stream was discarded.Fix:
agent-loop.js: synthetic skill-read assistant now carries a shortreasoning_content; force-final-summary capturestailThoughtsand attaches it to the persisted message.providers/index.jssanitizeMessages: defence-in-depth backfill. After the existing strip pass, only when the conversation already contains at least one assistant with non-emptyreasoning_content(i.e. thinking mode is "live" for this session), every other assistant without one is given a short placeholder. Scoped to reasoning-capable models only.Theme B —
skill-creatorhard-gate false positive (#146 follow-up)Symptom: in environments where no
skill-creatormeta-skill is installed, the model still followed the system-prompt instruction and fabricated askill_invoke({name:"skill-creator"})call, producing the confusing:Root cause: the post-#146 system-prompt language ("HARD GATE: you MUST call …") was unconditional; only the executor-side gate actually checks for installation.
Fix: three coordinated touch-ups so the gate is described and enforced consistently with what the executor does.
getProblemSolvingParadigm()now callsdiscoverSkills()and only emits the gate paragraph if askill-creator/skill_creator/skillcreatorvariant is actually installed.skill_createdescription softened from "HARD GATE" to "Quality gate — ONLY if a meta-skill named … is listed in the Available skills index"; explicit "do NOT fabricate askill_invokecall for it" when the index doesn't list one.skillInvokereturns a softNotice: …gate is inactive here…so the agent proceeds straight toskill_createinstead of hittingError: skill "…" not found.End result: when no meta-skill is installed, the gate is documented as inactive, the description tells the model not to fabricate, and the tool layer is forgiving if it does anyway.
Risk
isReasoning && anyHasReasoning— non-reasoning providers (OpenAI / Anthropic) are untouched. The placeholder is a single short string so it doesn't inflate token counts meaningfully.Test
deepseek-reasonerwith synthetic 32-iteration loop → no 400.skill-creatorinstalled → previously errored, now succeeds with the system prompt no longer instructing a fabricatedskill_invoke.Out of scope
No version bump in this PR; let the next release commit handle it.